Conversation
Greptile SummaryThis PR updates two tx stores — Key changes:
Minor issue found: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant RecheckMempool
participant CosmosTxStore
participant LegacyPool
participant TxStore
Note over Caller,TxStore: Replacement tx insert flow
Caller->>RecheckMempool: Insert(replacement tx @ nonce N)
RecheckMempool->>RecheckMempool: markTxInserted(tx)
RecheckMempool->>CosmosTxStore: InvalidateFrom(tx)
alt tx key exists in snapshot (is a replacement)
CosmosTxStore->>CosmosTxStore: remove all stored txs with nonce >= N
CosmosTxStore-->>RecheckMempool: removed > 0
Note over RecheckMempool: replacement NOT added to snapshot yet
else tx key not in snapshot (fresh insert)
CosmosTxStore-->>RecheckMempool: 0
RecheckMempool->>CosmosTxStore: AddTx(tx)
end
Caller->>LegacyPool: add(replacement tx)
LegacyPool->>LegacyPool: markTxRemoved(addr, old tx, Pending)
LegacyPool->>TxStore: RemoveTx(addr, old tx)
TxStore->>TxStore: RemoveTxsFromNonce(addr, nonce N)
Note over TxStore: removes old tx AND all txs with nonce >= N
LegacyPool->>LegacyPool: markTxReplaced(addr, replacement)
Note over LegacyPool: replacement NOT added to validPendingTxs
Note over Caller,TxStore: Next recheck cycle
RecheckMempool->>RecheckMempool: TriggerRecheck
RecheckMempool->>CosmosTxStore: rebuild snapshot via markTxRechecked
Note over CosmosTxStore: replacement + dependent txs re-added after validation
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…-snapshot-invalidation
| require.Equal(t, uint64(1), result[addr1][0].Tx.Nonce()) | ||
| } | ||
|
|
||
| func TestTxStoreConcurrentRemove(t *testing.T) { |
There was a problem hiding this comment.
not sure if this test makes sense to have anymore
|
|
||
| result := store.Txs(txpool.PendingFilter{}) | ||
| require.Len(t, result[addr1], 500) | ||
| require.Len(t, result[addr1], 0) |
There was a problem hiding this comment.
updating this bc i think the behavior we want now is if a tx is removed, all txs with a greater nonce should now be considered invalid and removed as well. they cannot be included in a block proposal.
|
@greptile re-review |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1074 +/- ##
==========================================
+ Coverage 65.45% 65.51% +0.06%
==========================================
Files 331 331
Lines 23262 23347 +85
==========================================
+ Hits 15225 15296 +71
+ Misses 6894 6887 -7
- Partials 1143 1164 +21
🚀 New features to boost your workflow:
|
|
@greptile re-review |
|
@greptile re-review |
mattac21
left a comment
mattac21
left a comment
There was a problem hiding this comment.
some questions but looks good to me
| // cosmosTxNonceMap extracts the signers from the transaction | ||
| // and returns a signer -> nonce map. | ||
| func cosmosTxNonceMap(tx sdk.Tx) (map[string]uint64, bool) { | ||
| signerSeqs, err := extractSignerSequences(tx) |
There was a problem hiding this comment.
is there a limit on the amount of signers a tx can have that is validated somewhere before this? if not then this is a user supplied value we are allocating memory on which isn't good (i.e. someone sends us a tx with 5 mil tiny invalid signers) (I can't comment on the line since it isn't changed, but extractSignerSequences line 487)
There was a problem hiding this comment.
mm i guess we are running ante handlers just before this, so that would probably be validated away in there
| if i > 0 { | ||
| b.WriteByte('|') | ||
| } | ||
| fmt.Fprintf(&b, "%s/%d", sig.account, sig.seq) |
There was a problem hiding this comment.
i guess same question on the number of max signers allowed here. could someone force us to allocate a massive string here? if so maybe we have to hash a key made of account + seq together with other signers keys in order to have a fixed sized key? but that seems way slower
| removed := 0 | ||
| nextTxs := make([]sdk.Tx, 0, len(s.txs)) | ||
| for _, existing := range s.txs { | ||
| if invalidatesCosmosTx(existing, thresholds) { |
There was a problem hiding this comment.
we compute the nonce map in invalidatedCosmosTx but we have already computed it in InvalidateFrom, could we pass it in?
There was a problem hiding this comment.
are you referring to the nonceMap on line 68? that one is the nonceMap for the tx we are checking, the one here is for each tx we check against
|
|
||
| // rebuild the txs list, skipping txs that are invalidated. | ||
| removed := 0 | ||
| nextTxs := make([]sdk.Tx, 0, len(s.txs)) |
There was a problem hiding this comment.
damn this feels super rough that we have to rebuild the entire list again. do you think it would be better if we changed the data structure of the tx store here to be more like the evm one which is a map? so we only have to rebuild per account
| s.txs = txs | ||
| s.keys = make(map[string]int, len(txs)) | ||
| for i, tx := range txs { | ||
| if key, ok := cosmosTxKey(tx); ok { |
There was a problem hiding this comment.
computing this key again seems really rough, would love if we could maybe cache it on the tx itself, kind of like how signatures are cached on eth transaction structs, but I agree probably fine to just leave this for now
3 participants
Description
updates the TxStore to handle tx replacements. txs that get replaced as a result of an insert should invalidate all same-sender txs with a higher nonce value, as they were dependent on the original tx's execution.
Closes: STACK-2479
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
mainbranch